Skip to content

Improve thread safety of Signal internal. #453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Improve thread safety of Signal internal. #453

wants to merge 1 commit into from

Conversation

@andersio
Copy link
Member

@andersio andersio commented Jun 14, 2017 •
edited
Loading

Basically the updateLock is removed, and state is wrapped with Atomic.

But why replacing the existing RCU scheme?

Let's look at how the compiled assembly does the work:

Thread A                           | Thread B
------------------------------------------------------------------
Lock `updateLock`.                 |
ARC Retain the new `AliveState`.   |
Pack it as a `State` enum.         |
Load the current `self.state`.     | Load `self.state`.
Store to `self.state`.             | Unpack the `State` enum.
Unpack the old `State` enum.       | Retain the `AliveState`.
ARC Release the old `AliveState`.  | Call the observers.
Unlock `updateLock`.               |

It seems safe, especially since writes are serialised.

But what if we shift the timeline of thread B a little bit further?

Thread A                           | Thread B
------------------------------------------------------------------
Lock `updateLock`.                 |
ARC Retain the new `AliveState`.   |
Pack it as a `State` enum.         |
Load the current `self.state`.     | 
Store to `self.state`.           <<!!!!!>> Load `self.state`.
Unpack the old `State` enum.       | Unpack the `State` enum.
ARC Release the old `AliveState`.<<!!!!!>> Retain the `AliveState`.
Unlock `updateLock`.               | Call the observers.

Data races now exist due to ARC not being part of the atomic read/write. Our RCU model considers only the atomicity of self.state alone without the hidden side effect of ARC.

While the contention window is extremely small (a few instruction cycles), it is possible on paper that the AliveState is released before a concurrent relaxed reader retains it.

So until we manage to find a way to use atomic with ARC, or having Swift natively provide an ARC aware AtomicPointer primitive, we should fallback to Atomic.

Checklist

  • Updated CHANGELOG.md.

@andersio andersio added this to the 2.0 milestone Jun 14, 2017
@andersio andersio changed the title Improve thread safety Signal internal. Improve thread safety of Signal internal. Jun 14, 2017
@andersio andersio force-pushed the signal-state-redo branch from 8df6b75 to a4317c6 Compare June 14, 2017 11:08
@andersio andersio force-pushed the signal-state-redo branch from a4317c6 to 88eb386 Compare June 14, 2017 12:58
@andersio andersio closed this Jun 15, 2017
@andersio andersio mentioned this pull request Jun 15, 2017
Merged
@andersio andersio deleted the signal-state-redo branch July 10, 2017 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug work in progress

Projects

None yet

Milestone

2.0

Development

Successfully merging this pull request may close these issues.

2 participants

@andersio